test: make the temp-folder and provider-guard checks safe under parallel runs - #277
Conversation
…s own The cleanup tests read the machine-wide temp root and counted entries by name prefix, so a staging folder another run was still using read as a leak. Each test now points the temp root at a folder inside its own workspace, where anything left over is its own. The extraction one was also filtering on a prefix the worker stopped using, so it counted zero on both sides and passed even with the cleanup removed. It now fails.
Both tests that check a context hook throws outside its provider now go through expectHookThrowsOutsideProvider, which catches the throw inside the component and asserts nothing reached console.error. The config one used to let the throw escape and silence the window error event it set off, which is the shape that can fail an unrelated file. useNotificationsContext had a guard that could not fire: the context carries a working no-op default, so the hook never sees an absent value. It is gone, and the reason it never threw is written where it was.
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Approved. This PR keeps the extraction cleanup assertions isolated from unrelated parallel runs. The tests point TMPDIR, TMP, and TEMP at a private root, both extraction implementations resolve staging through tmpdir(), and the tests verify that the root is empty after successful and refused extraction. The environment stubs are restored after each test.\n\nThe shared provider-guard helper catches the expected hook error inside renderHook, checks its type and message, and detects unexpected console errors. onTestFinished restores the spy when the test fails. The only runtime source change removes an unreachable guard from useNotificationsContext because its context has a non-null default value, so the guard could never fire.\n\nLocal verification passed: typecheck, lint with 0 errors and 15 existing warnings, format check, 1633 tests with 2 skipped, coverage at 92.59% statements / 89.83% branches / 92.03% functions / 94.07% lines, and build:unpack. GitHub typecheck, lint, test, SonarCloud, Ubuntu build, and Windows build checks also pass.
2 participants
Summary
Two test harness cleanups, one per commit, from #276 and #271. Nothing about how the launcher behaves changes.
The cleanup test in
tests/ipc/innoExtraction.test.tssnapshotted the machine-wide temp root, ran two extractions, and asserted the number of entries starting withriftlauncher-inno-had not moved. Three test files callrunInnoExtraction, they run in parallel, and each one's staging folder lives under that same root while it works, so a sibling mid-extraction across the snapshot counted as a leak in whichever file happened to be counting. Each of the two calls now gets a temp root inside the test's own workspace, throughvi.stubEnvonTMPDIR,TMPandTEMP, and the assertion is that the root is empty at the end. Nothing else writes there, so an empty folder means these two calls cleaned up after themselves and says nothing about anybody else.The archive extraction test next door had the same shape and one extra problem: it filtered on
vs-launcher-extract-, a prefix the worker stopped using, so it counted zero on both sides and could not fail. Same treatment, and it now means something.On the renderer side, the two tests that check a context hook throws outside its provider go through one helper,
expectHookThrowsOutsideProviderintests/renderer-dom/helpers/render.tsx. It is the shape #264 introduced: render the hook, catch the throw inside the component, check the message, and check nothing reachedconsole.error. It won over the config file's version because it does not depend on the window error machinery at all. The escaping form works today only because that test remembers to cancel the event and mute the log, and getting either wrong turns a render throw into an uncaught exception that fails whichever file was unlucky enough to be running. The helper carries the explanation, so the next one of these gets written by calling it rather than by copying a delicate arrangement.Those two are all of them. A grep across
tests/renderer-domfor the "must be used within" messages and forrenderHookunderexpect().toThrowturns up nothing else.The third item in #271, a guard test for
useNotificationsContext, turned out not to be writable. That context is created with a real no-op default value, souseContextnever hands the hook nothing and itsif (!context) throwcould not fire. Rather than leave an unreachable throw for the next person to try to cover, it is gone, with the reason the hook has no guard written where it was. That is the only production file in the branch and it changes no behaviour, since the branch was dead. Happy to split it out if you would rather keep this to tests only.Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
Reproducing #276 first. A loop running next to the suite keeps a rolling set of
/tmp/riftlauncher-inno-XXXXXXfolders alive, which is what the other two files callingrunInnoExtractionlook like from the outside. With that running against the old assertion,tests/ipc/innoExtraction.test.tsfailed 4 times out of 6, always on the temp folder case and never on anything else. With the new assertion under the same churn it passed 10 out of 10.Mutations, so the new assertions still bite. Disabling
removeSync(temporaryRoot)insrc/ipc/workers/innoExtraction.tsfails the test, naming the two folders left behind. Disabling the same line insrc/ipc/workers/extraction.tsfails the archive one now, where against the old prefix filter it stayed green, which is how I know that assertion had stopped doing anything. On the renderer side, rewriting the helper to let the throw escape the component fails both provider-guard tests on theconsole.errorcheck, which is the whole reason that check is there.Gates.
typecheckclean.lint:ci0 errors and 15 warnings, all pre-existing rendererreact-hooks/exhaustive-depsones.format:checkclean.test:coveragerun three times back to back, exit 0 each time, 137 files with 1,633 passed and 2 skipped, at 92.59% statements, 89.83% branches, 92.03% functions and 94.07% lines against floors of 87, 85, 85 and 89.build:unpackpasses on Linux x64. The only thing on stderr across those runs is jsdom's "Not implemented: Window's scrollTo() method" notice, which is ondevalready and comes from a component the DOM suite mounts.Related issues
Covers #276 and the first two parts of #271, with a note above on why the third part could not be written as asked.